-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bloom Compactor: Optimize check for fingerprint ownership #11389
Conversation
Trivy scan found the following vulnerabilities:
|
5b58326
to
cc5fe60
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary from the offline discussion between @chaudum and @vlad-diachenko The current implementation of getting the fingerprint ownership works generically no matter how many tokens an instance has in the ring, and therefore how many token ranges. At the moment, bloom compactors are configured with 1 token per instance, which means that there is always exactly 1 instance that has two token ranges (one from lastToken+1 to MaxUint32, and one from 0 to firstToken). There are two options then:
|
Discussed offline. I also think the second approach is going against the nature of ring. It is fragile to the changes in the number of tokens and requires future operators of the service to know the ring implementation. One of the advantages of the first approach as a result will create a job per range, which will nicely reduce job's responsibility. In second approach we better introduce a further divide up for the job, otherwise the work may get quite big. I'm happy for us to try and see both approaches. |
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
…nership Signed-off-by: Christian Haudum <[email protected]>
Handle ranges correctly when token is 0 or MaxUint32 Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
Divide full token range by the amount of instances to get equal token ranges. Assign ranges based on the sort order of the first token of an instance in the ring. Signed-off-by: Christian Haudum <[email protected]>
Signed-off-by: Christian Haudum <[email protected]>
be064b5
to
585c342
Compare
) Calling `c.sharding.OwnsFingerprint(tenant, uint64(fingerprint))` for each Series of a TSDB index is very expensive, because it not only creates the tenant's sub-ring but also needs to check the fingerprint against it. Instead, we can pre-calculate the current instance's token ranges and check if the (uint32 converted) fingerprint is contained within these ranges. Signed-off-by: Christian Haudum <[email protected]>
What this PR does / why we need it:
Calling
c.sharding.OwnsFingerprint(tenant, uint64(fingerprint))
for each Series of a TSDB index is very expensive, because it not only creates the tenant's sub-ring but also needs to check the fingerprint against it.Instead, we can pre-calculate the current instance's token ranges and check if the (uint32 converted) fingerprint is contained within these ranges.
Special notes for your reviewer:
The main change of this PR is 5b58326